[FEAT] Add bitstream module in lib_ccxr
#1649
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
src/lib_ccx/cc_bitstream.h
Outdated
| @@ -25,6 +24,9 @@ struct bitstream | |||
| // increased by the calling function, or not. | |||
| unsigned char *_i_pos; | |||
| int _i_bpos; | |||
| #ifndef DISABLE_RUST | |||
| void *rust_bs; // Pointer to Rust's Bitstream struct | |||
There was a problem hiding this comment.
Could we construct / deconstruct the struct when passing to rust instead of storing a different pointer?
There are other files, where you can see the to_ctype()
There was a problem hiding this comment.
yaa surely, this should be a better approach rather than having to maintain two structures. I will try to fix it in my next commit
There was a problem hiding this comment.
@prateekmedia , I have made changes in the bitstream.rs file , can you please review it now
Migration module changes
Minor Formatting Issue
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new bitstream module implemented in Rust and connects it to the existing C code via FFI.
- Introduces Rust-side bitstream functions and exposes them through the C wrapper
- Updates C bitstream implementation to delegate to Rust when
DISABLE_RUSTis not defined - Adjusts tests and build configuration to include the new module
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rust/wrapper.h | Includes the C header for bitstream to expose FFI |
| src/rust/src/libccxr_exports/mod.rs | Registers the bitstream export module |
| src/rust/lib_ccxr/src/util/bits.rs | Refactors parity tests to use assert! |
| src/rust/lib_ccxr/src/common/mod.rs | Adds bitstream to the common mod exports |
| src/rust/build.rs | Links the bitstream library into the build script |
| src/lib_ccx/cc_bitstream.c | Wraps the C implementation to call into Rust |
Comments suppressed due to low confidence (3)
src/lib_ccx/cc_bitstream.c:238
- The function signature returns
unsigned char *but the Rust FFIccxr_next_bytesexports aconst uint8_t *. Consider updating this toconst unsigned char *to preserve theconstqualifier and avoid discarding it.
bstr->bpos = 8;
src/rust/wrapper.h:14
- wrapper.h includes the C header for bitstream but does not declare the extern
ccxr_*functions provided by the Rust bitstream module, so consumers won’t see the Rust FFI symbols. Consider adding extern declarations for eachccxr_*function in this header.
#include "../lib_ccx/cc_bitstream.h"
src/rust/build.rs:30
- The
bitstreamentry may not match the actual C library name produced (likely something likeccx_bitstreamor part of the existingccxlibrary), which could cause linker errors. Verify that the linked library name matches the compiled archive.
"bitstream",
Bitstream: Removed redundant CType
prateekmedia
reviewed
Jul 6, 2025
…-flow bitstream: recommended changes for is_byte_aligned
prateekmedia
reviewed
Jul 6, 2025
Comment on lines
8
to
16
| /// # Safety | ||
| /// This function is unsafe because it: | ||
| /// - Dereferences a raw pointer (`bitstream_ptr`) without null checking | ||
| /// - Assumes `bitstream_ptr` points to a valid, properly initialized and aligned `bitstream` struct | ||
| /// - Assumes the `bitstream` struct has sufficient memory allocated for all fields | ||
| /// - Assumes `rust_bitstream` is properly initialized with valid data | ||
| /// - Performs pointer arithmetic on `rust_bitstream.data` assuming it points to valid memory | ||
| /// - The caller must ensure the `bitstream` struct remains valid for the duration of the operation | ||
| /// - The data referenced by `rust_bitstream.data` must remain valid during the operation |
There was a problem hiding this comment.
Keep Safety comments concise to 1 or 2 lines at max.
…-removal bitstream: recommended changes for long comments
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
steel-bucket
added a commit
to steel-bucket/ccextractor
that referenced
this pull request
Jul 29, 2025
* feat: Add bitstream module * run code formatters * Run cargo clippy --fix * Run cargo fmt --all * refactor: remove rust pointer from C struct * feat: Add bitstream module * run code formatters * Run cargo clippy --fix * Run cargo fmt --all * refactor: remove rust pointer from C struct * Added Bitstream to libccxr_exports * Minor Formatting Issue * Bitstream: Removed redundant CType * bitstream: recommended changes for is_byte_aligned * bitstream: recommended changes for long comments * bitstream: comment fix * bitstream: removed redundant comparism comments --------- Co-authored-by: Deepnarayan Sett <[email protected]> Co-authored-by: Deepnarayan Sett <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
This PR migrates the bitstream functionality from C to Rust while maintaining backward compatibility by creating a new Rust implementation in
lib_ccxr/src/common/bitstream.rsand connecting it to the existing C code through FFI bindings.